Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(service-worker): fix LruList 'remove' bug #22769

Closed
wants to merge 1 commit into from
Closed

fix(service-worker): fix LruList 'remove' bug #22769

wants to merge 1 commit into from

Conversation

manda-linda
Copy link

remove method not removing url from state.map

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #22768

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@kara kara added the area: service-worker Issues related to the @angular/service-worker package label Mar 14, 2018
gkalpak
gkalpak reviewed Mar 14, 2018
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I suspect this (possibly together with #22325 (comment)) would also fix #22218.
Since #22325 seems to be stale, would you mind setting previous/next to null for remove nodes (just in case 😁).

@manda-linda
Copy link
Author

Updated the PR setting node.previous/next to null before deleting. The change in PR #22325 shouldn't be necessary now since it was mostly a bandaid for this bug.

gkalpak
gkalpak reviewed Mar 15, 2018
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.pop() also needs clearing .next/.previous.
Or maybe we could make .pop() just call .remove() and avoid duplication:

pop(): string|null {
  const url = this.state.tail;
  if (url !== null) {
    this.remove(url);
  }
  return url;
}

packages/service-worker/worker/src/data.ts Outdated
this.state.count--;
return true;
}

// The node is not the head, so it has a previous. It may or may not be the tail.
// If it is not, then it has a next. First, grab the previous node.
const previous = this.state.map[node.previous !] !;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😢

packages/service-worker/worker/src/data.ts Outdated
@@ -145,14 +145,14 @@ class LruList {
const next = this.state.map[node.next !] !;
next.previous = null;
this.state.head = next.url;
delete this.state.map[url];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set node.next to null here too?

@gkalpak gkalpak mentioned this pull request Mar 15, 2018
Closed
3 tasks
@gkalpak
Copy link
Member

gkalpak commented Mar 15, 2018

BTW, could you also add the following at the end of the commit message:

Fixes #22218
Fixes #22768

@manda-linda manda-linda force-pushed the lru-remove branch 3 times, most recently from a9e92a8 to 751fb2b Compare March 16, 2018 03:57
gkalpak
gkalpak approved these changes Mar 20, 2018
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit. LGTM otherwise.

packages/service-worker/worker/src/data.ts Outdated
@@ -207,6 +194,7 @@ class LruList {
// The next pointer of the node being inserted gets set to the old head, before the head
// pointer is updated to this node.
node.next = this.state.head;
node.previous = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant here. It is either a new node (so previous is already null) or it is remove()'s job to set .previous to null.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, yeah I guess you're right. I ran across this issue earlier where the head had a previous node, but that must have been before the switch to using 'remove'.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Mar 20, 2018
fix(service-worker): fix LruList bugs
ff40c03 
'remove' method not removing url from state.map
'accessed' method not removing 'previous' reference from existing  node when it becomes the head

Fixes angular#22218
Fixes angular#22768
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 21, 2018
@IgorMinar IgorMinar closed this in 8c2a578 Mar 21, 2018
IgorMinar pushed a commit that referenced this pull request Mar 21, 2018
@IgorMinar
fix(service-worker): fix LruList bugs (#22769)
65f8943 
'remove' method not removing url from state.map
'accessed' method not removing 'previous' reference from existing  node when it becomes the head

Fixes #22218
Fixes #22768

PR Close #22769
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@leo6104
fix(service-worker): fix LruList bugs (angular#22769)
14c7edc 
'remove' method not removing url from state.map
'accessed' method not removing 'previous' reference from existing  node when it becomes the head

Fixes angular#22218
Fixes angular#22768

PR Close angular#22769
@gkalpak gkalpak mentioned this pull request Apr 15, 2018
Closed
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: service-worker Issues related to the @angular/service-worker package cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@manda-linda @gkalpak @kara @googlebot